dotnet: allow customizing resources in subclasses #400
Conversation
There was a problem hiding this comment.
@gasparnagy what is the broader usecase for customization?
For example, to apply custom styling, I imagine that an additional resource would be needed. For that use case I'm not sure overriding would be the best solution. For more complicated customization, I imagine it would be better to fork the entire project.
And would it be possible to prefer composition over inheritance? I.e. provide the constructor with three functions to lazy-load the resources as needed. Using a rather ad-hoc override makes API evolution much more difficult.
|
@mpkorstanje Yes, the motivation was getting towards to enable customizations like mentioned at https://github.com/cucumber/react-components?tab=readme-ov-file#styling. You are absolutely right, this change is really just a very basic way of customization - replace the resources with an own-compiled ones. This is basically to be able to make some experiments until the proper customization model/interfaces are figured out. So this is not going to be the advertised way of customization. As this component is now going to be used in Reqnroll, you would need to replace quite many components in order to apply a quick fix if you would fork this. That would be a blocker for many early adapters. |
Then I'd still prefer a solution that doesn't use extension. IIRC .Net supports default values constructor parameters so the solution I suggested would be pretty light weight and effectively allow for the same experimentation with customization. |
Ok. I will rework. |
|
@mpkorstanje What do you think? |
|
I'm a bit swamped this week. Hopefully next week. |
Thx. This is kind of a blocker for us, so if you happen to have a bit of time earlier to review that it is appreciated. |
| /// </summary> | ||
| /// <param name="name">The resource name</param> | ||
| /// <returns>The resource content</returns> | ||
| protected string GetResource(string name) |
There was a problem hiding this comment.
This can be private now I think?
| /// <summary> | ||
| /// Interface for providing resources to the HTML formatter | ||
| /// </summary> | ||
| public interface IResourceProvider |
There was a problem hiding this comment.
Can we indicate that his is an experimental API? If we're planning to do a settings object, with builder I imagine use of that object would be something like,
new Setting()
.withCssResource(...)
.withJavascriptResource(...)
.withEct(...)
And with sensible defaults in place if a particular method of the builder isn't called.
The IResourceProvider doesn't support those sensible defaults and so I imagine it will be phased out again too.
There was a problem hiding this comment.
Highlighted that this is experimental
| /// Gets the HTML template | ||
| /// </summary> | ||
| /// <returns>The HTML template</returns> | ||
| string GetTemplateResource(); |
There was a problem hiding this comment.
I'm not sure if the template is suitable for replacement. It is not actually a mustache template, it just looks like one.
There is a hard coded assumption in the code that the template contains {{css}}, {{messages}} and {{script}} and also in that order. If that ever changes, we'd break consumers of the API.
But that is acceptable if this is marked as an experimental API.
There was a problem hiding this comment.
Currently I need this for the tweaking, but the interface is marked as experimental. But you are right, in long term, we should not need this.
The theming customization might require something, but we can solve it in other ways too - let's discuss when we talk about the customizations.
|
Superseded by #409 |
🤔 What's changed?
Made resource-loading methods virtual, so that they can be overridden in subclasses
⚡️ What's your motivation?
Allowing generating customized HTML reports.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.